Feat: add support for initializing vecs client with custom schema #63
Conversation
majamil16
changed the title
olirice
left a comment
olirice
left a comment
There was a problem hiding this comment.
nice job on the PR
did you see this comment here:
#62 (comment)
if we add a top level parameter for schema in the Client it'll be much more difficult to support defining a schema on the Collection when that inevitably gets requested.
Would you be up for refactoring to make this a per Collection keyword only argument?
Note that it'd be a bit more involved since the Metadata instance can't be bound to a single schema in that case
|
@olirice thanks for the feedback! sure, I'm definitely up to implement that change. |
update - move support for schema onto collection instead of client
|
hey @olirice, made some updates. let me know your thoughts, thanks! |
|
looks great. if we can address those last few concerns this will merge |
|
I'm looking forward to this PR being merged! |
|
hey, thanks @olirice for the review! will finish this up this week |
updates based on feedback
| Creates a client from a Postgres connection string and optional schema. | ||
| Defaults to `vecs` schema. |
There was a problem hiding this comment.
and optional schema. Defaults to `vecs` schema.
can be removed
| - id: black | ||
| language_version: python3.9 | ||
| language_version: python3.8 |
There was a problem hiding this comment.
what was the rationale for this change?
| import vecs | ||
|
|
||
| PYTEST_DB = "postgresql://postgres:password@localhost:5611/vecs_db" | ||
| PYTEST_SCHEMA = "test_schema" |
There was a problem hiding this comment.
the best way to test that escaping is done correctly in all places is to use a crazy schema name.
I tested basic operations with the schema name "esCape Me!" and the test suite fails.
To reproduce that, try:
foo: Collection = client.get_or_create_collection(name="foo", schema="esCape Me!", dimension=5)
and you'll get
sqlalchemy.exc.ProgrammingError: (psycopg2.errors.InvalidSchemaName) schema ""esCape Me!"" does not exist
|
Hi Guys ! Any news about it ? |
|
Hi - any news? For me personally, this change would greatly improve my experience using |
|
Hey @olirice I can take and finish this if @majamil16 is not going to? |
|
thanks! but lets hold off until we land the other PR you have open |
6 participants
What kind of change does this PR introduce?
Feature: add support for connecting to a user-specified database schema (instead of hard-coding
vecs).This was accomplished by adding the
schemaargument to the Client class and replacing all hard-coded references to schemavecs.Also added
schemaparameter tocreate_clientmethod. For backwards compatibility, it still defaults tovecs.What is the current behavior?
Currently, the client defaults to schema
vecs. Users may want to place vecs collections under a schema according to their own naming conventions.Addresses #62
Please link any relevant issues here.
What is the new behavior?
Users can now instantiate a Client instance with a custom schema. For example:
Additional context
Add any other context or screenshots.
Test coverage attached
